Skip to content

field movement layer improvements#3720

Open
a-png129 wants to merge 21 commits into
UBC-Thunderbots:masterfrom
a-png129:alice/field_movement_layer_improvements
Open

field movement layer improvements#3720
a-png129 wants to merge 21 commits into
UBC-Thunderbots:masterfrom
a-png129:alice/field_movement_layer_improvements

Conversation

@a-png129
Copy link
Copy Markdown
Contributor

@a-png129 a-png129 commented May 19, 2026

Description

  • Added drag-to-orient movement control
    • Shift+Alt+click begins a movement command
    • Dragging sets the robot's final orientation
    • Releasing sends the move command with the selected orientation
    • Instead of defaulting to facing downward, the default orientation is the selected robot's current orientation
  • Added robot position preview on hover
    • While holding Shift+Alt, hovering over the field displays a preview of the target robot position and orientation before committing the command
  • Added selected robot indicator
    • The currently selected robot is now highlighted with an outline that follows its live position and orientation

Testing Done

Manual testing in Thunderscope
(the frame rate only looks this bad in my recording... it's fine otherwise)

Screencast.from.2026-05-20.23-01-05.webm

Resolved Issues

#3719

Review Checklist

It is the reviewers responsibility to also make sure every item here has been covered

  • Function & Class comments: All function definitions (usually in the .h file) should have a javadoc style comment at the start of them. For examples, see the functions defined in thunderbots/software/geom. Similarly, all classes should have an associated Javadoc comment explaining the purpose of the class.
  • Remove all commented out code
  • Remove extra print statements: for example, those just used for testing
  • Resolve all TODO's: All TODO (or similar) statements should either be completed or associated with a github issue

@a-png129 a-png129 changed the title Alice/field movement layer improvements field movement layer improvements May 19, 2026
@a-png129 a-png129 linked an issue May 19, 2026 that may be closed by this pull request
3 tasks

# This must be enabled for the mouse_moved_in_scene_signal to be emitted
self.detect_mouse_movement_in_scene = False
self.detect_mouse_movement_in_scene = True
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I needed this turned on to implement the robot preview on hover. Previously, it was only turned on when the measure tool was in use. I'm not sure if keeping this always on would be a problem. If so, maybe we could implement some sort of counter so it's only turned off if neither the measure tool nor the field movement layer are enabled?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I added this flag because detecting mouse movements every tick was hurting performance.

# Enable/disable detect_mouse_movement_in_scene in ExtendedGLViewWidget
# so that the mouse_in_scene_moved_signal is emitted if measure mode is on.
#
# Normally we want to disable detect_mouse_movement_in_scene so that we
# don't do unnecessary calculations every tick to find the point in the scene
# that the mouse is pointing at.
self.gl_view_widget.detect_mouse_movement_in_scene = self.measure_mode_enabled

We can get rid of the flag, seems like it was just premature optimization. I haven't noticed any extra lag when measure mode is on, at least not on my PC

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea I agree it should be fine to make this change, but perhaps if there is a way to just use a boolean to check if field movement layer is enabled that would be best

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added the boolean! Now when a layer or the measure tool is enabled/disabled, it checks to see if any other layers/tools require mouse movements and changes the flag accordingly

@a-png129 a-png129 marked this pull request as ready for review May 21, 2026 06:53
Copy link
Copy Markdown
Member

@williamckha williamckha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is sweet, thanks alice!

Comment thread src/software/thunderscope/gl/layers/gl_movement_field_test_layer.py Outdated
Comment thread src/software/thunderscope/gl/layers/gl_movement_field_test_layer.py Outdated

# This must be enabled for the mouse_moved_in_scene_signal to be emitted
self.detect_mouse_movement_in_scene = False
self.detect_mouse_movement_in_scene = True
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I added this flag because detecting mouse movements every tick was hurting performance.

# Enable/disable detect_mouse_movement_in_scene in ExtendedGLViewWidget
# so that the mouse_in_scene_moved_signal is emitted if measure mode is on.
#
# Normally we want to disable detect_mouse_movement_in_scene so that we
# don't do unnecessary calculations every tick to find the point in the scene
# that the mouse is pointing at.
self.gl_view_widget.detect_mouse_movement_in_scene = self.measure_mode_enabled

We can get rid of the flag, seems like it was just premature optimization. I haven't noticed any extra lag when measure mode is on, at least not on my PC

Copy link
Copy Markdown
Member

@nycrat nycrat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey Alice I tested this PR and everything looks good except one minor issue about when the robot preview visual is updated (see review comment)

Comment on lines +181 to +185
def mouse_in_scene_moved(self, event: MouseInSceneEvent) -> None:
"""Handle mouse moved events to update robot position preview.

:param event: The event
"""
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The overlay for the target robot position only shows up after the mouse has been dragged, and only disappears once the mouse moves.

Perhaps add a key press handler to set the target_point whenever shift+alt is pressed so it will updated immediately.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did try adding an alt key press handler but it had some unreliable behaviour. When I only pressed and released alt, it worked fine. But when I pressed shift first and then alt, it didn't register. There was some similar weirdness with the key release, depending on the order of events. I figured it was simpler to just leave it at mouse movements than introduce something unreliable


# This must be enabled for the mouse_moved_in_scene_signal to be emitted
self.detect_mouse_movement_in_scene = False
self.detect_mouse_movement_in_scene = True
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea I agree it should be fine to make this change, but perhaps if there is a way to just use a boolean to check if field movement layer is enabled that would be best

Copy link
Copy Markdown
Contributor

@Andrewyx Andrewyx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good so far!

Comment thread src/software/thunderscope/gl/layers/gl_movement_field_test_layer.py Outdated
Comment thread src/software/thunderscope/gl/layers/gl_movement_field_test_layer.py Outdated
Comment thread src/software/thunderscope/gl/layers/gl_movement_field_test_layer.py Outdated
@Andrewyx
Copy link
Copy Markdown
Contributor

@a-png129 bumping this ticket for updates, since we will need it for testing soon

@a-png129
Copy link
Copy Markdown
Contributor Author

So sorry about the delay!! I'll make sure to address the comments today

@a-png129
Copy link
Copy Markdown
Contributor Author

@williamckha @nycrat @Andrewyx ready for another review 🫡

Copy link
Copy Markdown
Contributor

@Andrewyx Andrewyx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CI seems to be failing, and pending eric's comment, but otherwise lgtm

@@ -1,134 +0,0 @@
#
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure what triggered this deletion

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#3564 again...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Field Test Layer Add Hover

4 participants